Added ability to use regex rules #133

cognitivegears · 2021-08-12T20:51:25Z

Please check if the PR fulfills these requirements

[ X ] The commit message follows our guidelines
[ X ] Tests for the changes have been added (for bug fixes / features)
[ X ] Docs have been added / updated (for bug fixes / features) see docs/README.md

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
New feature

What is the current behavior? (You can also link to an open issue here)
Currently there is no way to specify a regular expression in a rule. This means that some rules are difficult to create, for example:

Smart word boundaries on multi word terms, such as "Black List" matching even with a tab or other charcter (e.g. "black\s*list")
A rule that matches "master" without word boundaries in order to match both master and words composed with master (i.e. IsMasterRepo), but excluding specific words, like "masters". (e.g. "master(?:\b|[^s])")
Many others... With regular expressions, flexible rules can be created to match many different cases

What is the new behavior (if this is a feature change)?
Added a new option "regex_terms" on the rules to specify that they should be treated as regular expressions. If set to yes, then terms are not escaped before adding to the terms list and errors displayed / rules set to ignore from the regular expression compile on error.

Does this PR introduce a breaking change? (What changes might users need to make due to this PR?)
No

Other information:
N/A

codecov · 2021-08-12T20:52:42Z

Codecov Report

Merging #133 (e3fdd00) into main (1fe097d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #133      +/-   ##
==========================================
+ Coverage   95.90%   95.92%   +0.01%     
==========================================
  Files          21       21              
  Lines         708      711       +3     
==========================================
+ Hits          679      682       +3     
  Misses         18       18              
  Partials       11       11

Impacted Files	Coverage Δ
pkg/rule/rule.go	`94.54% <100.00%> (+0.15%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fe097d...e3fdd00. Read the comment docs.

caitlinelfring · 2021-08-13T00:09:14Z

I've been wanting this feature, but the schema I had in mind was something like this:

rules:
  - name: foo
    terms:
      - foo
      - Foo
  - name: bar
    regex: "(?i)bar"

# when both are defined, `terms` wins, `regex` would be ignored, maybe log debug message that both are defined?
  - name: baz
    terms:
      - baz
      - BaZ
    regex: "(?i)baz"

caitlinelfring · 2021-08-13T00:10:24Z

docs/rules.md

+
+* If `true`, terms will be evaluated as regular expressions
+* If `false`, terms will be treated as plain-text values
+* **NOTE** this is an advanced feature. Rules will be skipped if they do not compile. Only use non-capturing groups in patterns. Look-around assertions are not supported.


I think a fatal error should happen if a supplied regex can't compile. Otherwise the user may never know that their rule isn't being used.

Sure - I added a Error log message, so when it runs if a rule won't compile, an error is generated but it continues, it looks like this:

2021-08-13T09:03:27-05:00 ERR Unable to compile regular expression, disabling rule error="error parsing regexp: missing closing ): `(?i)(black(\\s*list)`" Rule=rulename

I thought about letting it do a fatal error instead, though I chose this approach in anticipation of the remote ruleset feature - my thought was that if there is a rule that doesn't work in a remote ruleset that the user can't necessarily directly change, it could continue on after printing the error. That said, I'm totally happy to go either way with this - if you want it a fatal error that's easy to change, I can just change it back to a MustCompile instead, or doing a panic after logging the error so that they still get that debug info from that log. What do you prefer here?

my thought was that if there is a rule that doesn't work in a remote ruleset that the user can't necessarily directly change

I think we should tackle this when it becomes a problem or concern.

cognitivegears · 2021-08-13T14:02:16Z

I've been wanting this feature, but the schema I had in mind was something like this:

rules:
  - name: foo
    terms:
      - foo
      - Foo
  - name: bar
    regex: "(?i)bar"

# when both are defined, `terms` wins, `regex` would be ignored, maybe log debug message that both are defined?
  - name: baz
    terms:
      - baz
      - BaZ
    regex: "(?i)baz"

No problem, should be pretty straightforward. I'll change this.

cognitivegears · 2021-08-13T14:23:11Z

I've been wanting this feature, but the schema I had in mind was something like this:

rules:
  - name: foo
    terms:
      - foo
      - Foo
  - name: bar
    regex: "(?i)bar"

# when both are defined, `terms` wins, `regex` would be ignored, maybe log debug message that both are defined?
  - name: baz
    terms:
      - baz
      - BaZ
    regex: "(?i)baz"

No problem, should be pretty straightforward. I'll change this.

Oh - one more question. I see the (?i) in your example. I assume that means that you are intending that this wouldn't do the wrapping at all that it does for terms, so word_boundary, word_boundary_start, and word_boundary_end would also be ignored? That makes sense, but in that case should that also then put a debug message if any of those are defined just to make it clear what is happening?

caitlinelfring · 2021-08-18T11:53:58Z

I assume that means that you are intending that this wouldn't do the wrapping at all that it does for terms, so word_boundary, word_boundary_start, and word_boundary_end would also be ignored? That makes sense, but in that case should that also then put a debug message if any of those are defined just to make it clear what is happening?

That's correct, if you supply regex, no other options that are used to generate the regex internally would be used. regex should be a "use at your own risk" config setting.

cognitivegears · 2021-09-05T19:15:03Z

Sorry for the delay - updated the code to follow the suggested changes above.

GitHub Action run 1, commit: 1fe097d

cognitivegears · 2021-09-05T19:45:59Z

I see that the benchmark tool is failing. I'm not sure why that would be, however, unless there is some instability in the benchmarking process itself. This change should not represent any runtime change in performance, and only a single additional conditional when compiling rules. If I'm missing something there though please let me know so that I can help resolve.

caitlinelfring · 2021-09-28T13:08:27Z

I see that the benchmark tool is failing. I'm not sure why that would be, however, unless there is some instability in the benchmarking process itself. This change should not represent any runtime change in performance, and only a single additional conditional when compiling rules. If I'm missing something there though please let me know so that I can help resolve.

Apologies for not responding for so long! The benchmark test is failing because it's supposed to create a comment on the PR with the results, but the github action that handles the PR commenting doesn't work on forks. I'm disabling that part in #147.

Aside from that, looking at the output of the benchmark tool (which just runs go test -bench on the branch and compares it to the results on main), seems like BenchmarkRootRunE has higher ns/op. Would you be able to look into why?

Puneethgr · 2023-02-21T06:38:20Z

@cognitivegears Please let me know if there are any updates. This is a nice feature.

simskij · 2023-03-27T16:19:48Z

@caitlinelfring we're using this quite extensively at Canonical, and really need this feature. anything i could assist you with to get this through?

Added ability to use regex rules

0b6ae6b

Removed extra line on markdown

7814e7f

caitlinelfring reviewed Aug 13, 2021

View reviewed changes

Changed regex to use preferred format

c8e59af

github-actions bot and others added 3 commits September 5, 2021 19:21

docs: autogenerated update for docs/snippets/woke.md: 1fe097

8401b28

GitHub Action run 1, commit: 1fe097d

Resolved merge conflicts

0e51dd9

Resolved markdownlint

e3fdd00

cognitivegears mentioned this pull request Sep 10, 2021

Apply different rules to different paths/files #137

Open

caitlinelfring mentioned this pull request Oct 11, 2022

Question: Is there a way to allow "ansible-test sanity" but disallow "sanity"? #234

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ability to use regex rules #133

Added ability to use regex rules #133

cognitivegears commented Aug 12, 2021

codecov bot commented Aug 12, 2021 •

edited

Loading

caitlinelfring commented Aug 13, 2021

caitlinelfring Aug 13, 2021

cognitivegears Aug 13, 2021 •

edited

Loading

caitlinelfring Aug 18, 2021

cognitivegears commented Aug 13, 2021

cognitivegears commented Aug 13, 2021

caitlinelfring commented Aug 18, 2021

cognitivegears commented Sep 5, 2021

cognitivegears commented Sep 5, 2021

caitlinelfring commented Sep 28, 2021

Puneethgr commented Feb 21, 2023 •

edited

Loading

simskij commented Mar 27, 2023

Added ability to use regex rules #133

Are you sure you want to change the base?

Added ability to use regex rules #133

Conversation

cognitivegears commented Aug 12, 2021

codecov bot commented Aug 12, 2021 • edited Loading

Codecov Report

caitlinelfring commented Aug 13, 2021

caitlinelfring Aug 13, 2021

Choose a reason for hiding this comment

cognitivegears Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

caitlinelfring Aug 18, 2021

Choose a reason for hiding this comment

cognitivegears commented Aug 13, 2021

cognitivegears commented Aug 13, 2021

caitlinelfring commented Aug 18, 2021

cognitivegears commented Sep 5, 2021

cognitivegears commented Sep 5, 2021

caitlinelfring commented Sep 28, 2021

Puneethgr commented Feb 21, 2023 • edited Loading

simskij commented Mar 27, 2023

codecov bot commented Aug 12, 2021 •

edited

Loading

cognitivegears Aug 13, 2021 •

edited

Loading

Puneethgr commented Feb 21, 2023 •

edited

Loading